Include frontend with oss query-service #7247
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗 |
|
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to bcd62bc in 1 minute and 0 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/query-service/Dockerfile:27
- Draft comment:
Ensure the target directory exists before copying. Docker often creates it, but explicitly creating the directory may improve clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
- Docker's COPY command automatically creates target directories if they don't exist. 2. Creating directories explicitly is not a common practice in Dockerfiles unless there's a specific permission/ownership requirement. 3. The comment starts with "Ensure that..." which violates our rules. 4. The comment doesn't point out an actual problem - it's just a style suggestion that doesn't improve functionality.
The comment might have merit if there were specific permission requirements for the web directory that needed to be set before copying.
Even if directory permissions were a concern, they could be set after the COPY command. The current approach is standard Docker practice.
Delete the comment as it starts with "Ensure that...", doesn't identify a real problem, and suggests an unnecessary change to standard Docker practices.
2. pkg/query-service/Dockerfile:26
- Draft comment:
Document that copying the frontend build fixes the missing web directory issue (feat(ee): serve frontend pages with query-service #6696). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. pkg/query-service/Dockerfile:27
- Draft comment:
Consider adding a permission adjustment (chmod/chown) for /etc/signoz/web/ if query-service needs non-root access. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_TddtpkEENbEBXcjD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Hi @davispuh, Apart from the changes mentioned here, we also need to change the corresponding github workflows for this to happen. We are in the process of revamping our CI and we plan to undertake this there itself. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
#6696 updated
ee/query-service/Dockerfilebut didn't updatepkg/query-service/DockerfileThis causes
query-servicefail to start when usingosstagged images.Workaround is to pass
SIGNOZ_WEB_ENABLED=falseenv but it's not obvious because previously it just worked.Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Fixes
query-servicestartup failure by including frontend directory inpkg/query-service/Dockerfile.pkg/query-service/Dockerfile, addedCOPY frontend/build/ /etc/signoz/web/to include the frontend directory in theosstagged images.query-servicedue to missing frontend directory when usingosstagged images.SIGNOZ_WEB_ENABLED=falseis no longer necessary.This description was created by
for bcd62bc. It will automatically update as commits are pushed.